Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GitHub/fix programmatic rename #6179

Merged
merged 13 commits into from
May 26, 2020
Merged

GitHub/fix programmatic rename #6179

merged 13 commits into from
May 26, 2020

Conversation

ocallesp
Copy link
Contributor

@ocallesp ocallesp commented May 11, 2020 •
edited
Loading

Fixes #5139, #5436

What this PR does:

  • Updates the package version for Microsoft.VisualStudio.LanguageServices and Microsoft.CodeAnalysis to use the new Roslyn API RenameDocumentAsync and UpdateSolutionAsync.
  • replaces Renamer.cs with RenamerProjectTreeActionHandlers.cs

Brief description of the changes:
The new Roslyn API does much of the work done in Renamer.cs like check if there are symbol to rename, rename symbols, and notify to other VS features. This helps dotnet Project System to move code to Roslyn and delete Renamer.cs.

Part of the code of Renamer.cs was moved RenamerProjectTreeActionHandler to be executed before CPS and before the file rename. At this point the api RenameDocumentAsync can get old document and can also check if the file rename is programmatic or not.

RenamerProjectTreeActionHandler does in chronological order:

  1. Check if programmatic or user rename.
  2. RenameDocumentAsync produces the actions.
  3. Check if actions are feasible.
  4. Rename file (remove old document and add new document)
  5. We prompt to rename symbols
  6. Wait for Language Service to see the rename (HandleRename)
  7. Execute the actions UpdateSolutionAsync

What is missing:

  • Fix Unit tests. This will be done after the code review.

Future work:

Microsoft Reviewers: Open in CodeFlow

@ocallesp ocallesp requested a review from a team as a code owner May 11, 2020 05:41
davidwengier
davidwengier approved these changes May 11, 2020
View reviewed changes
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you work around the CreatePkgDef issue?

@ocallesp ocallesp force-pushed the github/fix_programmatic_rename branch 2 times, most recently from ac8d14f to 7304ec8 Compare May 11, 2020 22:35
@ocallesp
Copy link
Contributor Author

How did you work around the CreatePkgDef issue?
Can you elaborate more on this ? I see CreatePkgDef is an utility that creates a .pkgdef file.
Do you mean when renaming the .pkgdef file ?

@davidwengier
Copy link
Member

When we talked earlier in the week, updating Microsoft.VisualStudio.Threading caused a build error with CreatePkgDef I thought. If that has gone away, thats great!

@ocallesp ocallesp force-pushed the github/fix_programmatic_rename branch 5 times, most recently from 14d1a90 to bd400fe Compare May 13, 2020 01:01
@davkean davkean mentioned this pull request May 21, 2020
Open
@davkean
Copy link
Member

davkean commented May 21, 2020

As we spoke offline, we played around this and we discovered the following issues:

  1. If I rename a item just by case say from 'foo.cs' -> 'Foo.cs', I get:
---------------------------
Microsoft Visual Studio
---------------------------
Sequence contains no elements
---------------------------
OK   
---------------------------
  1. If I rename a item when the file name doesn't match the name of type, I get:
---------------------------
Microsoft Visual Studio
---------------------------
Sequence contains no elements
---------------------------
OK   
---------------------------
  1. If I rename a file to include characters that not valid name characters (such as a space), we actually go ahead and try to rename it to include that

@ryzngard
Copy link
Contributor

As we spoke offline, we played around this and we discovered the following issues:

  1. If I rename a item just by case say from 'foo.cs' -> 'Foo.cs', I get:
---------------------------
Microsoft Visual Studio
---------------------------
Sequence contains no elements
---------------------------
OK   
---------------------------
  1. If I rename a item when the file name doesn't match the name of type, I get:
---------------------------
Microsoft Visual Studio
---------------------------
Sequence contains no elements
---------------------------
OK   
---------------------------
  1. If I rename a file to include characters that not valid name characters (such as a space), we actually go ahead and try to rename it to include that

  1. is expected, it behaves the same as inline rename + file rename behavior. If we should change this let's discuss.

  2. Seems correct right? We don't want to update the type name to the new name unless it already matched?

  3. Seems like a good candidate for returning an error for GetErrors() on the action

@davkean
Copy link
Member

davkean commented May 21, 2020

The errors are not expected, we should be handling this correctly and not failing the rename of the file itself which we are doing.

@ryzngard
Copy link
Contributor

Not sure I follow, but may be missing context. The API isn't returning errors, is it errors that happen because an empty actionset is returned?

@davkean
Copy link
Member

davkean commented May 21, 2020

I just grabbed his branch, installed it and started playing around with the behavior, we show dialogs with those errors when I perform those actions. I'm not referring to to what the API is returning.

ocalles and others added 13 commits May 25, 2020 22:32
@ocalles
Update Roslyn Nuget packages to use a new APIs
f168e1f 
New APIs:
- RenameDocumentAsync
- UpdateSolutionAsync
@ocalles
Remove the partial class Renamer that implements IFileRenameHandler
d0011cd 
The functionality is now implemented by
RenamerProjectTreeActionHandler.cs which uses the new Roslyn APIs.
@ocalles
Code cleanup and fix warnings
9ba629a 
Updated Microsoft.CSharp to 4.7.0 and fixed null pointer warnings.

Workaround for CreatePkgDef
This change fixes the broken build.
Microsoft.VisualStudio.Threading took a dependency on Bcl.Interfaces.
@ocalles
Code cleanup
2615d3d
@davkean @ocalles
Remove package references to right layer
e7ce61c
@ocalles
Fix new warning messages detected by the updated package version of A…
2e33d29 
…nalyzers
@ocalles
Fix file rename bugs
63b56b3 
1) 'foo.cs' -> 'Foo.cs'.
2) File name doesn't match the name of type.
@ocalles
Fix file rename should not rename types when invalid characters.
b318055
@ocalles
Code cleanup unit test for file renames.
108dc57
@ocalles
Check special characters before file rename
45f91a8
@ocalles
Change position of CPSRenameAsync() to always call it.
9ecd19c
@ocalles
Code cleanup
f05ae54
@ocalles
Refactor to extract methods from RenameAsync
1a24a92
@ocallesp ocallesp force-pushed the github/fix_programmatic_rename branch from edc2c6d to 1a24a92 Compare May 26, 2020 05:43
@ocallesp ocallesp merged commit 04bcccf into dotnet:master May 26, 2020
@ghost ghost added this to the 16.7 milestone May 26, 2020
@ryzngard ryzngard mentioned this pull request Jun 19, 2020
Closed
@ocallesp ocallesp deleted the github/fix_programmatic_rename branch October 14, 2020 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drewnoakes drewnoakes drewnoakes left review comments

@ryzngard ryzngard ryzngard left review comments

@davidwengier davidwengier davidwengier approved these changes

@davkean davkean davkean left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
16.7
Development

Successfully merging this pull request may close these issues.

Undoing a Roslyn rename prompts a rename
6 participants
@ocallesp @davidwengier @davkean @ryzngard @drewnoakes @ocalles